refactor(engine): extract shared slug + folder helpers into slug-utils#35
Merged
Merged
Conversation
Consolidates 4+ duplicated helpers that had accumulated across the
gitops engine as symptom-fixes piled up. Pure factoring, zero
behavior change at every reachable call site.
Duplications collapsed:
- `slugify` — 4 byte-identical copies (pull.ts, dep-dedup.ts,
audit.ts, setup.ts) → 1 in src/slug-utils.ts
- `extractBaseSlug` — 2 byte-identical copies → 1
- `FOLDER_MAP` — 2 byte-identical copies (pull.ts, resources.ts) → 1
- `UUID_SUFFIX_RE` — open-coded in 3 places → 1 constant
- recanonicalize's inlined precondition-2 check (UUID prefix match)
→ extracted as `isEngineSuffixedSlug`
src/slug-utils.ts is config-free by design (no `./config.ts` import,
no side effects at load) so it's safely importable from any test
without priming process.argv / VAPI_TOKEN. This is the testability
property the prior dep-dedup.ts comment claimed for its local
duplicates but didn't actually enforce.
Regex tightening: shared `UUID_SUFFIX_RE` uses `^(.+)-([0-9a-f]{8})$`
(non-empty base) where the prior pull/dep-dedup copies used
`^(.*)-...` (allowed empty base). Strict improvement — engine-
generated keys always have a non-empty base, and the only input
class affected is the synthetic `-<8hex>` shape which is never
produced by `generateResourceId`. Pinned by a regression test in
tests/slug-utils.test.ts.
Back-compat: dep-dedup.ts re-exports slugify/extractBaseSlug so
existing tests importing them via that path keep working.
Tests: 228/228 pass (208 prior + 20 new slug-utils cases covering
slugify behavior, UUID_SUFFIX_RE boundaries, extractBaseSlug loose
form, isEngineSuffixedSlug strict form).
This was referenced May 15, 2026
dhruva-reddy
added a commit
that referenced
this pull request
May 15, 2026
…e-fns into one generic helper (#34) * refactor(engine): extract shared slug + folder helpers into slug-utils (#35) Consolidates 4+ duplicated helpers that had accumulated across the gitops engine as symptom-fixes piled up. Pure factoring, zero behavior change at every reachable call site. Duplications collapsed: - `slugify` — 4 byte-identical copies (pull.ts, dep-dedup.ts, audit.ts, setup.ts) → 1 in src/slug-utils.ts - `extractBaseSlug` — 2 byte-identical copies → 1 - `FOLDER_MAP` — 2 byte-identical copies (pull.ts, resources.ts) → 1 - `UUID_SUFFIX_RE` — open-coded in 3 places → 1 constant - recanonicalize's inlined precondition-2 check (UUID prefix match) → extracted as `isEngineSuffixedSlug` src/slug-utils.ts is config-free by design (no `./config.ts` import, no side effects at load) so it's safely importable from any test without priming process.argv / VAPI_TOKEN. This is the testability property the prior dep-dedup.ts comment claimed for its local duplicates but didn't actually enforce. Regex tightening: shared `UUID_SUFFIX_RE` uses `^(.+)-([0-9a-f]{8})$` (non-empty base) where the prior pull/dep-dedup copies used `^(.*)-...` (allowed empty base). Strict improvement — engine- generated keys always have a non-empty base, and the only input class affected is the synthetic `-<8hex>` shape which is never produced by `generateResourceId`. Pinned by a regression test in tests/slug-utils.test.ts. Back-compat: dep-dedup.ts re-exports slugify/extractBaseSlug so existing tests importing them via that path keep working. Tests: 228/228 pass (208 prior + 20 new slug-utils cases covering slugify behavior, UUID_SUFFIX_RE boundaries, extractBaseSlug loose form, isEngineSuffixedSlug strict form). * refactor(push): extract reconcileStateKeyForResource — fold two 94-line ensure-fns into one generic helper `ensureToolExists` and `ensureStructuredOutputExists` were structurally identical 94-line functions differing only in: resource type label, apply function, state section, remote-list cache, and the per-type bookkeeping array. Both implemented the same dedup-then-apply flow: look up existing dashboard/state match by canonical name, adopt with orphan-deletion guard, mark `touched` for `mergeScoped`, call apply. Behavioral contract preserved exactly: - Log strings byte-identical (verified path-by-path against pre-refactor) - `autoApplied.add` BEFORE the `if (!uuid) return` early-exit - `applied[type]++`, `pushToAutoAppliedList`, `touched.add` AFTER the null check — preserves dry-run / drift-halt semantics - Orphan-deletion guard scope unchanged: deletes state keys pointing at the adopted UUID, leaves `duplicateUuids` alone for `npm run cleanup` to handle - try/catch boundary identical - All 228 prior tests pass unchanged, including the integration test in tests/push-dry-run.test.ts push.ts shrunk -129 net LOC (two 94-line functions collapsed to ~14-line wrappers). Helper is `tools | structuredOutputs` narrow today; adding a future type requires a deliberate union widening + LABELS map entry, not a config flag. `vapiEnv` and `formatError` parameters are required (not optional with placeholder defaults) so a future caller can't accidentally emit a degraded warning or error message. Tests: 244/244 pass (228 prior + 16 new — 8 scenarios × 2 resource types covering happy path, ambiguous match, null applyFn ordering contract, orphan-deletion guard scope, run-scoped idempotency, state-hit/dashboard-hit/no-match branches). Closes the symptom-fix pattern documented in improvements.md #10 (now handled by the generic helper instead of the two hardcoded functions).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Pure factoring PR. Consolidates duplicated helpers that accumulated across the gitops engine as symptom-fixes piled up. Zero behavior change at every reachable call site. Tier 1 of an engine consolidation series — surfaced by the post-mortem on the recanonicalize PR (#32, base of this stack).
What gets deduplicated
Why a separate module instead of "just import from pull.ts"
`src/slug-utils.ts` is config-free by design — no `./config.ts` import, no side effects at load. `config.ts` asserts `argv[2]` / `VAPI_TOKEN` at module load and `process.exit(1)`s under unit tests. Any test that imports a slug helper transitively via `pull.ts` would otherwise have to prime `process.argv` and `process.env.VAPI_TOKEN` (see `tests/recanonicalize.test.ts:7-8`).
The prior `dep-dedup.ts` comment claimed its local duplicates were "intentionally copied for testability" but `new-file-gate.ts` imported the helper from `pull.ts` anyway — so the testability claim didn't actually hold. The new `slug-utils.ts` makes it real: zero imports, importable from any test without ceremony.
Two-form contract: loose vs. strict
The helpers split into two semantically distinct forms that callers were previously mixing:
Regex tightening (intentional)
Shared `UUID_SUFFIX_RE` uses `^(.+)-([0-9a-f]{8})$` (non-empty base). Prior pull/dep-dedup copies used `^(.*)-...` (allowed empty base). Strict improvement:
Files changed
Net: +275 / -85 → +190. New file weight is mostly the test suite (170 lines) — source-code net is +105 / -85 = +20.
Test plan
Code review
In-branch review by code-reviewer subagent. No blocking findings. Four L-level cleanups addressed in-branch:
Stack
This is PR 1 of 2 in the engine-consolidation series. PR 2 stacks on this one and folds `ensureToolExists` / `ensureStructuredOutputExists` (both ~94 lines, structurally identical) into a generic `reconcileStateKeyForResource` helper. Reviewable alone — PR 2 is sequencing, not technical dependency.
Related
Note: this PR supersedes #33, which was auto-closed by GitHub when its base branch (the stack parent, now merged as #32) was deleted on merge.